Fix up usermod libArchive settings #4669
Conversation
The ConfigureProjectLibBuilder process will flush and reload the library settings from the on-disk manifests if any new library is installed at that stage. This has the side effect of reverting the libArchive setting applied to usermods which was performed prior to that call. Apply the setting afterwards, instead. Fixes wled#4597
|
""" WalkthroughThis change refactors usermod handling in the build system and usermod scripts, standardizing how usermods are included and validated during the build. It introduces a post-build validation script to ensure all selected usermods are linked, updates build configuration files to disable library archiving, and adjusts usermod setup scripts to reference dependencies via Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pio-scripts/load_usermods.py (3)
10-14: Minor cleanup: remove redundantf-string and avoid shadowingThe
fprefix is unnecessary because no interpolation occurs, and re-using the loop variable namefforPathobjects may confuse future readers.- all_usermods = [f for f in usermod_dir.iterdir() if f.is_dir() and f.joinpath('library.json').exists()] - env.GetProjectConfig().set(f"env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods])) + all_usermods = [ + mod_path + for mod_path in usermod_dir.iterdir() + if mod_path.is_dir() and mod_path.joinpath("library.json").exists() + ] + env.GetProjectConfig().set("env:usermods", "custom_usermods", + " ".join([mod_path.name for mod_path in all_usermods]))🧰 Tools
🪛 Ruff (0.8.2)
11-11: Undefined name
env(F821)
14-14: Undefined name
env(F821)
14-14: f-string without any placeholders
Remove extraneous
fprefix(F541)
16-33: Consolidate repeated path checks infind_usermodThe current implementation repeats almost identical logic three times. A data-driven approach is easier to maintain and extend.
-def find_usermod(mod: str) -> Path: - """Locate this library in the usermods folder. - We do this to avoid needing to rename a bunch of folders; - this could be removed later - """ - # Check name match - mp = usermod_dir / mod - if mp.exists(): - return mp - mp = usermod_dir / f"{mod}_v2" - if mp.exists(): - return mp - mp = usermod_dir / f"usermod_v2_{mod}" - if mp.exists(): - return mp - raise RuntimeError(f"Couldn't locate module {mod} in usermods directory!") +def find_usermod(mod: str) -> Path: + """ + Locate a library folder inside *usermods* using a few common naming + conventions. + """ + candidates = [ + mod, + f"{mod}_v2", + f"usermod_v2_{mod}", + ] + for candidate in candidates: + mp = usermod_dir / candidate + if mp.exists(): + return mp + raise RuntimeError(f"Couldn't locate module {mod!r} in usermods directory")
34-38: Improveis_wled_modulerobustness
- Case-insensitive prefix check avoids missing modules named
WLED-XYZ.- Explicit equality check catches the rare case where
dep.src_diris exactlyusermod_dir.- Using
os.fspathensures comparison between the same path types.-from pathlib import Path # For OS-agnostic path manipulation +from pathlib import Path +import os @@ -def is_wled_module(dep: LibBuilderBase) -> bool: - """Returns true if the specified library is a wled module - """ - return usermod_dir in Path(dep.src_dir).parents or str(dep.name).startswith("wled-") +def is_wled_module(dep: LibBuilderBase) -> bool: + """ + Return *True* if the dependency resides inside *usermods* **or** + its declared name starts with ``wled-`` (case-insensitive). + """ + src_path = Path(dep.src_dir).resolve() + return ( + src_path == usermod_dir.resolve() + or usermod_dir.resolve() in src_path.parents + or str(dep.name).lower().startswith("wled-") + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/load_usermods.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
pio-scripts/load_usermods.py
8-8: Undefined name env
(F821)
11-11: Undefined name env
(F821)
14-14: Undefined name env
(F821)
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
🔇 Additional comments (1)
pio-scripts/load_usermods.py (1)
5-5:❓ Verification inconclusive
Verify availability of
LibBuilderBasein the current PlatformIO release
platformio.builder.tools.piolib.LibBuilderBaseis an internal class whose import path has changed in the past (e.g. 6.1 → 6.1.6). A mismatched version silently breaks the build at import-time.To confirm that the symbol exists in the CI environment, you can grep the installed PlatformIO package:
If this fails, fall back to
platformio.builder.tools.piolib_builder(the new location in ≥6.1.17).
🏁 Script executed:
#!/bin/bash python - <<'PY' import importlib, inspect, sys try: m = importlib.import_module("platformio.builder.tools.piolib") print("LibBuilderBase found in", m.__file__) except ModuleNotFoundError as e: sys.exit("❌ LibBuilderBase not found: " + str(e)) PYLength of output: 68
Verify availability of
LibBuilderBaseimport in your CI environmentThe sandbox test did not have the
platformiopackage installed, so we couldn’t confirm whether
platformio.builder.tools.piolib.LibBuilderBaseexists in your CI. Please verify on your CI runners by executing:platformio --version python3 - <<'PY' import importlib, sys for module in ( "platformio.builder.tools.piolib", "platformio.builder.tools.piolib_builder" ): try: m = importlib.import_module(module) print(f"✅ LibBuilderBase found in {module} → {m.__file__}") sys.exit(0) except ImportError: continue sys.exit("❌ LibBuilderBase not found in either location") PYIf the only available location is
piolib_builder, update your import to fallback gracefully:-from platformio.builder.tools.piolib import LibBuilderBase +try: + from platformio.builder.tools.piolib import LibBuilderBase +except ImportError: + from platformio.builder.tools.piolib_builder import LibBuilderBase
pio-scripts/load_usermods.py
Outdated
| for dep in result.depbuilders: | ||
| if is_wled_module(dep): | ||
| # Add the wled folder to the include path | ||
| dep.env.PrependUnique(CPPPATH=wled_dir) | ||
| # Add WLED's own dependencies | ||
| for dir in extra_include_dirs: | ||
| dep.env.PrependUnique(CPPPATH=dir) | ||
| # Enforce that libArchive is not set; we must link them directly to the executable | ||
| if dep.lib_archive: | ||
| build = dep._manifest.get("build", {}) | ||
| build["libArchive"] = False | ||
| dep._manifest["build"] = build |
There was a problem hiding this comment.
Ensure path types match & persist libArchive override
PrependUnique treats Path and str as distinct objects; duplicates slip through and
some SCons versions can’t handle Path. Also update the in-memory flag so later logic
(e.g. verbose prints) remains accurate.
- # Add the wled folder to the include path
- dep.env.PrependUnique(CPPPATH=wled_dir)
+ # Add the WLED root folder to the include path
+ dep.env.PrependUnique(CPPPATH=str(wled_dir))
@@
- dep.env.PrependUnique(CPPPATH=dir)
+ dep.env.PrependUnique(CPPPATH=str(dir))
@@
- if dep.lib_archive:
+ if dep.lib_archive:
build = dep._manifest.get("build", {})
build["libArchive"] = False
dep._manifest["build"] = build
+ # Keep the runtime flag in sync with the manifest change
+ dep.lib_archive = False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for dep in result.depbuilders: | |
| if is_wled_module(dep): | |
| # Add the wled folder to the include path | |
| dep.env.PrependUnique(CPPPATH=wled_dir) | |
| # Add WLED's own dependencies | |
| for dir in extra_include_dirs: | |
| dep.env.PrependUnique(CPPPATH=dir) | |
| # Enforce that libArchive is not set; we must link them directly to the executable | |
| if dep.lib_archive: | |
| build = dep._manifest.get("build", {}) | |
| build["libArchive"] = False | |
| dep._manifest["build"] = build | |
| for dep in result.depbuilders: | |
| if is_wled_module(dep): | |
| # Add the WLED root folder to the include path | |
| dep.env.PrependUnique(CPPPATH=str(wled_dir)) | |
| # Add WLED's own dependencies | |
| for dir in extra_include_dirs: | |
| dep.env.PrependUnique(CPPPATH=str(dir)) | |
| # Enforce that libArchive is not set; we must link them directly to the executable | |
| if dep.lib_archive: | |
| build = dep._manifest.get("build", {}) | |
| build["libArchive"] = False | |
| dep._manifest["build"] = build | |
| # Keep the runtime flag in sync with the manifest change | |
| dep.lib_archive = False |
There was a problem hiding this comment.
The point about PrependUnique is well taken, I'll make a correction there.
dep.lib_archive is a read-only @property derived from the contents of dep._manifest; it cannot be directly set.
Also, it's poor practice to put unrelated review items in a single comment. If you find yourself using 'also' to begin a sentence, that's strong indication that it would be best to make a second comment instead.
There was a problem hiding this comment.
@willmmiles Thank you for the feedback! I appreciate you correcting me on dep.lib_archive - I should have verified it was a writable property before suggesting the direct assignment.
I'll also take your advice about separating unrelated review items into different comments in future reviews. Using "also" is indeed a good indicator that comments should be split.
🐰✏️ Rabbit taking notes thoughtfully
✏️ Learnings added
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: In PlatformIO's LibBuilderBase class, `lib_archive` is a read-only @property derived from the contents of the `_manifest` dictionary and cannot be directly set.
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When reviewing code, separate unrelated items into different comments rather than combining them. If using "also" to begin a sentence in a review comment, that's a strong indication it should be a separate comment.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Ensure that entries put in CPPPATH are always strings so SCons can correctlly deduplicate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pio-scripts/load_usermods.py (1)
14-14: Minor: Remove unnecessary f-string prefixThe f-string prefix is unnecessary when there are no placeholders in the string.
- env.GetProjectConfig().set(f"env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods])) + env.GetProjectConfig().set("env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods]))🧰 Tools
🪛 Ruff (0.8.2)
14-14: Undefined name
env(F821)
14-14: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/load_usermods.py(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
pio-scripts/load_usermods.py (1)
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.799Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
🪛 Ruff (0.8.2)
pio-scripts/load_usermods.py
8-8: Undefined name env
(F821)
11-11: Undefined name env
(F821)
14-14: Undefined name env
(F821)
14-14: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (5)
pio-scripts/load_usermods.py (5)
5-5: Good addition of LibBuilderBase importThis import is appropriately added for the new type annotation in the is_wled_module function, keeping dependencies organized at the top of the file.
10-13: Good refactoring of all_usermods initializationMoving the initialization of
all_usermodsinto the conditional block is a good practice as it limits the variable scope to where it's actually used.🧰 Tools
🪛 Ruff (0.8.2)
11-11: Undefined name
env(F821)
16-17: Improved type safety with return annotationAdding the return type annotation
-> Pathto the find_usermod function improves code clarity and enables better type checking.
34-37: Good utility function abstractionThe new
is_wled_modulefunction effectively centralizes the logic for identifying WLED modules, making the code more maintainable and easier to understand.
108-119: Proper fix for the libArchive setting issueThis implementation correctly addresses the issue described in #4597 by ensuring the libArchive setting is applied after the ConfigureProjectLibBuilder process completes, and by properly converting Path objects to strings as needed.
|
I tried this but unfortunately, it does not fix the issue when using VScode. |
yes i can confirm that it did not work for me too |
|
Fie! Back to the drawing board.. :) |
This reverts commit 0d44e7e.
Not all of them were removed by the reverted commit; re-do the rest of them.
Rather than try and fail to add this property, abort if it's missing from any requested usermod.
|
let me know if I need to run a test |
@DedeHai Please be my guest! My local testing hasn't been very effective at finding problems - "it works for me" has been the mantra here all along. :( |
|
I did play with the latest commit yesterday, can't say it improves the problem, but also does not make it worse: its still more or less luck if a UM gets properly added. Sometimes I see the green line in the compile log of all usermods being added, but they still do not end up in the binary. |
|
Can you please walk me through your test sequence (environments, changes, etc?) I'm going to try adding a bunch of validations next, to see if I can trap where things differ from one build to the next. I think cache poisoning might still be a problem, too - I might need to force a linker cache epoch break in addition to making build file changes. |
|
its not much of a test sequence but here is what I tried:
On my PC (which probably is NOT the cleanest PIO environment) its very rare that the UMs get properly compiled into the bin. As mentioned before: if built by powershell command line, it worked every time I tested that. Let me know if you need more details or if you have a receipe I should follow |
|
hi, i also tried it yesterday and it didnt work for me. in my tests it didnt work for i.e.
the result is always the same. But when i do exactly the same and use the command i posted here #4597 (comment) it just works. It doesnt matter if i use an older pc where windows 10 was installed 1 year ago, or a fresh installed windows 10/11, so for me all vs code on windows always causes that problem. |
|
I wonder why it's only on Windows we are seeing this. It's not something silly like a filepath separator or case insensitivity bug is it? |
Add additional validation of the linker .map output to confirm that the correct usermods were added.
Neither the info panel nor the settings dialog can be trusted to accurately report the usermod list: - Not all usermods necessarily add to the info panel - Not all usermods necessarily add to the config page - wled#4609 is required for the config page to be correct Add a short list to the info object that lists the loaded usermod IDs. This is not displayed via the UI, but can be queried with curl or web debug tools. To be removed when usermod loading is working well.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pio-scripts/validate_usermods.py (2)
1-7: Clean up unused imports.There are two imported modules that aren't being used in the code.
-import sys -from platformio import util🧰 Tools
🪛 Ruff (0.11.9)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
6-6:
platformio.utilimported but unusedRemove unused import:
platformio.util(F401)
31-35: Consider more robust detection of usermod table entries.This function looks for a specific string in the map file. For more reliability, consider using a regex pattern to match the usermod table entries, similar to the approach used in
check_map_file_objects.-def count_registered_usermods(map_file: list[str]) -> int: - """ Returns the number of usermod objects in the usermod list """ - # Count the number of entries in the usermods table section - return len([x for x in map_file if ".dtors.tbl.usermods.1" in x]) +def count_registered_usermods(map_file: list[str]) -> int: + """ Returns the number of usermod objects in the usermod list """ + # Count the number of entries in the usermods table section + pattern = re.compile(r"\.dtors\.tbl\.usermods\.\d+") + return len([x for x in map_file if pattern.search(x)])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pio-scripts/validate_usermods.py(1 hunks)platformio.ini(1 hunks)wled00/um_manager.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wled00/um_manager.cpp (1)
wled00/fcn_declare.h (11)
addToJsonInfo(458-458)obj(422-422)obj(422-422)obj(423-423)obj(423-423)obj(424-424)obj(424-424)obj(425-425)obj(425-425)obj(426-426)obj(426-426)
🪛 Ruff (0.11.9)
pio-scripts/validate_usermods.py
2-2: sys imported but unused
Remove unused import: sys
(F401)
6-6: platformio.util imported but unused
Remove unused import: platformio.util
(F401)
91-91: Undefined name Import
(F821)
92-92: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (4)
platformio.ini (1)
119-119: Good addition for usermod validation.Adding the post-build validation script is a good approach to ensure usermods are properly linked in the final binary, addressing the core issue with inconsistent usermod inclusion.
wled00/um_manager.cpp (1)
42-48: Enhanced JSON output with usermod IDs - excellent for diagnostics.The addition of a nested array containing all usermod IDs will make it much easier to diagnose which usermods are actually loaded at runtime. This complements the validation script nicely by providing runtime verification capabilities.
pio-scripts/validate_usermods.py (2)
13-30: Well-structured validation function.The
check_map_file_objectsfunction is well-implemented, using regex to identify object files from usermod directories in the map file.
91-92: The PlatformIO environment setup looks good.Using
AddPostActionto trigger validation after the ELF file is built is the correct approach.🧰 Tools
🪛 Ruff (0.11.9)
91-91: Undefined name
Import(F821)
92-92: Undefined name
env(F821)
Difference direction was inverted. It's tough to test when it always works correctly on your local machine! H/t @coderabbitai
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
pio-scripts/validate_usermods.py (7)
1-6: Cleanup unused imports.The imports
sys(line 2) andplatformio.util(line 6) are not used anywhere in the code.import re -import sys from pathlib import Path # For OS-agnostic path manipulation from click import secho from SCons.Script import Action, Exit -from platformio import util🧰 Tools
🪛 Ruff (0.11.9)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
6-6:
platformio.utilimported but unusedRemove unused import:
platformio.util(F401)
8-11: Missing return type annotation.The function
read_linesis missing its return type annotation. For consistency with the other functions, add-> list[str]to the function signature.-def read_lines(p: Path): +def read_lines(p: Path) -> list[str]: """ Read in the contents of a file for analysis """ with p.open("r", encoding="utf-8", errors="ignore") as f: return f.readlines()
31-34: Consider adding error handling for missing table section.The function assumes that
.dtors.tbl.usermods.1will always be in the map file if usermods are present. Consider adding a warning if this section is not found at all, which could indicate a more fundamental build issue.def count_registered_usermods(map_file: list[str]) -> int: """ Returns the number of usermod objects in the usermod list """ # Count the number of entries in the usermods table section - return len([x for x in map_file if ".dtors.tbl.usermods.1" in x]) + entries = [x for x in map_file if ".dtors.tbl.usermods.1" in x] + if not entries and any("usermod" in x.lower() for x in map_file): + # If no entries found but "usermod" is mentioned, warn about possible section naming issue + secho("WARNING: No entries found in .dtors.tbl.usermods.1 section, but usermod references exist", fg="yellow") + return len(entries)
36-36: Remove extra blank line.There's an extra blank line here that isn't necessary.
- def validate_map_file(source, target, env):
40-40: Fix spacing in string concatenation.There's an extra space after the division operator that should be removed.
- map_file_path = build_dir / env.subst("${PROGNAME}.map") + map_file_path = build_dir / env.subst("${PROGNAME}.map")
47-49: Consider adding error handling for empty usermods.The code should handle the case where
custom_usermodsis an empty string, which would result in a list with one empty string element after splitting.- usermods = env.GetProjectOption("custom_usermods","").split() + usermods = [um for um in env.GetProjectOption("custom_usermods","").split() if um] libdeps = env.GetProjectOption("lib_deps", []) lib_builders = env.GetLibBuilders()
1-92: The script addresses the PR objectives well but consider Windows-specific handling.This validation script effectively ensures that usermod libraries are correctly linked into the final build, which addresses part of the issues mentioned in the PR objectives. However, considering the feedback from users about persistent issues on Windows with VSCode, you might want to add specific handling for Windows environments or diagnostic information to help troubleshoot Windows-specific issues.
I can help design additional diagnostic features for Windows environments if needed, such as detecting file path issues, case sensitivity problems, or VSCode-specific behaviors mentioned in the PR comments.
🧰 Tools
🪛 Ruff (0.11.9)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
6-6:
platformio.utilimported but unusedRemove unused import:
platformio.util(F401)
91-91: Undefined name
Import(F821)
92-92: Undefined name
env(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pio-scripts/validate_usermods.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
pio-scripts/validate_usermods.py
2-2: sys imported but unused
Remove unused import: sys
(F401)
6-6: platformio.util imported but unused
Remove unused import: platformio.util
(F401)
91-91: Undefined name Import
(F821)
92-92: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (4)
pio-scripts/validate_usermods.py (4)
13-29: Good implementation of regex-based object file checking.The function correctly builds a regex pattern to match object files from usermod directories and efficiently checks for their presence in the map file. The approach of using a compiled regex for performance and the clear handling of matched directories is commendable.
53-74: Good implementation of usermod resolution logic.The code properly maps usermod names to library dependencies and builders, with appropriate error handling for missing entries. This approach ensures that all usermods are properly accounted for before proceeding with validation.
82-88: Correctly implemented missing usermods detection.The code correctly uses
set(usermod_dirs).difference(confirmed_usermods)to identify usermods that are specified but not found in the linked output, resolving the issue mentioned in past review comments.
91-92:✅ Verification successful
Understand that
Importandenvare PlatformIO special keywords.The static analysis tools flag
Importandenvas undefined, but these are special constructs in PlatformIO's build system.Importis a directive that loads an environment variable, andenvis the imported environment. This is standard in PlatformIO scripts.
🏁 Script executed:
#!/bin/bash # Check if Import is a common pattern in other PlatformIO scripts echo "Checking for Import pattern in other PlatformIO scripts:" rg -l "Import\(\"env\"\)" --type pyLength of output: 215
Import/env usage is correct and consistent with other PlatformIO scripts
Verified that
Import("env")appears in multiple PlatformIO build scripts (e.g.,validate_usermods.py,build_ui.py), confirming this is the standard pattern. No changes are required—please disregard static‐analysis warnings about undefined names in this context.🧰 Tools
🪛 Ruff (0.11.9)
91-91: Undefined name
Import(F821)
92-92: Undefined name
env(F821)
pio-scripts/validate_usermods.py
Outdated
| import re | ||
| import sys | ||
| from pathlib import Path # For OS-agnostic path manipulation | ||
| from click import secho | ||
| from SCons.Script import Action, Exit | ||
| from platformio import util | ||
|
|
||
| def read_lines(p: Path): | ||
| """ Read in the contents of a file for analysis """ | ||
| with p.open("r", encoding="utf-8", errors="ignore") as f: | ||
| return f.readlines() | ||
|
|
||
| def check_map_file_objects(map_file: list[str], usermod_dirs: list[str]) -> set[str]: | ||
| """ Checks that an object file from each usermod_dir appears in the linked output | ||
|
|
||
| Returns the (sub)set of usermod_dirs that are found in the output ELF | ||
| """ | ||
| # Pattern to match symbols in object directories | ||
| # Join directories into alternation | ||
| usermod_dir_regex = "|".join([re.escape(dir) for dir in usermod_dirs]) | ||
| # Matches nonzero address, any size, and any path in a matching directory | ||
| object_path_regex = re.compile(r"0x0*[1-9a-f][0-9a-f]*\s+0x[0-9a-f]+\s+\S+/(" + usermod_dir_regex + r")/\S+\.o") | ||
|
|
||
| found = set() | ||
| for line in map_file: | ||
| matches = object_path_regex.findall(line) | ||
| for m in matches: | ||
| found.add(m) | ||
| return found | ||
|
|
||
| def count_registered_usermods(map_file: list[str]) -> int: | ||
| """ Returns the number of usermod objects in the usermod list """ | ||
| # Count the number of entries in the usermods table section | ||
| return len([x for x in map_file if ".dtors.tbl.usermods.1" in x]) | ||
|
|
||
|
|
||
| def validate_map_file(source, target, env): | ||
| """ Validate that all usermods appear in the output build """ | ||
| build_dir = Path(env.subst("$BUILD_DIR")) | ||
| map_file_path = build_dir / env.subst("${PROGNAME}.map") | ||
|
|
||
| if not map_file_path.exists(): | ||
| secho(f"ERROR: Map file not found: {map_file_path}", fg="red", err=True) | ||
| Exit(1) | ||
|
|
||
| # Load project settings | ||
| usermods = env.GetProjectOption("custom_usermods","").split() | ||
| libdeps = env.GetProjectOption("lib_deps", []) | ||
| lib_builders = env.GetLibBuilders() | ||
|
|
||
| secho(f"INFO: Expecting {len(usermods)} usermods: {', '.join(usermods)}") | ||
|
|
||
| # Map the usermods to libdeps; every usermod should have one | ||
| usermod_dirs = [] | ||
| for mod in usermods: | ||
| modstr = f"{mod} = symlink://" | ||
| this_mod_libdeps = [libdep[len(modstr):] for libdep in libdeps if libdep.startswith(modstr)] | ||
| if not this_mod_libdeps: | ||
| secho( | ||
| f"ERROR: Usermod {mod} not found in build libdeps!", | ||
| fg="red", | ||
| err=True) | ||
| Exit(1) | ||
| # Save only the final folder name | ||
| usermod_dir = Path(this_mod_libdeps[0]).name | ||
| # Search lib_builders | ||
| this_mod_builders = [builder for builder in lib_builders if Path(builder.src_dir).name == usermod_dir] | ||
| if not this_mod_builders: | ||
| secho( | ||
| f"ERROR: Usermod {mod} not found in library builders!", | ||
| fg="red", | ||
| err=True) | ||
| Exit(1) | ||
| usermod_dirs.append(usermod_dir) | ||
|
|
||
| # Now parse the map file | ||
| map_file_contents = read_lines(map_file_path) | ||
| confirmed_usermods = check_map_file_objects(map_file_contents, usermod_dirs) | ||
| usermod_object_count = count_registered_usermods(map_file_contents) | ||
|
|
||
| secho(f"INFO: {len(usermod_dirs)}/{len(usermods)} libraries linked via custom_usermods, producing {usermod_object_count} usermod object entries") | ||
| missing_usermods = set(usermod_dirs).difference(confirmed_usermods) | ||
| if missing_usermods: | ||
| secho( | ||
| f"ERROR: No object files from {missing_usermods} found in linked output!", | ||
| fg="red", | ||
| err=True) | ||
| Exit(1) | ||
| return None | ||
|
|
||
| Import("env") | ||
| env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", Action(validate_map_file, cmdstr='Checking map file...')) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling for failed libdep resolution.
The code will exit if a usermod is not found in libdeps or library builders, but it doesn't handle the case where a usermod is resolved to multiple libdeps. This could happen if there are multiple libraries with the same name.
modstr = f"{mod} = symlink://"
this_mod_libdeps = [libdep[len(modstr):] for libdep in libdeps if libdep.startswith(modstr)]
- if not this_mod_libdeps:
+ if len(this_mod_libdeps) == 0:
secho(
f"ERROR: Usermod {mod} not found in build libdeps!",
fg="red",
err=True)
Exit(1)
+ elif len(this_mod_libdeps) > 1:
+ secho(
+ f"ERROR: Usermod {mod} resolves to multiple libdeps: {this_mod_libdeps}!",
+ fg="red",
+ err=True)
+ Exit(1)
# Save only the final folder name
usermod_dir = Path(this_mod_libdeps[0]).name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import re | |
| import sys | |
| from pathlib import Path # For OS-agnostic path manipulation | |
| from click import secho | |
| from SCons.Script import Action, Exit | |
| from platformio import util | |
| def read_lines(p: Path): | |
| """ Read in the contents of a file for analysis """ | |
| with p.open("r", encoding="utf-8", errors="ignore") as f: | |
| return f.readlines() | |
| def check_map_file_objects(map_file: list[str], usermod_dirs: list[str]) -> set[str]: | |
| """ Checks that an object file from each usermod_dir appears in the linked output | |
| Returns the (sub)set of usermod_dirs that are found in the output ELF | |
| """ | |
| # Pattern to match symbols in object directories | |
| # Join directories into alternation | |
| usermod_dir_regex = "|".join([re.escape(dir) for dir in usermod_dirs]) | |
| # Matches nonzero address, any size, and any path in a matching directory | |
| object_path_regex = re.compile(r"0x0*[1-9a-f][0-9a-f]*\s+0x[0-9a-f]+\s+\S+/(" + usermod_dir_regex + r")/\S+\.o") | |
| found = set() | |
| for line in map_file: | |
| matches = object_path_regex.findall(line) | |
| for m in matches: | |
| found.add(m) | |
| return found | |
| def count_registered_usermods(map_file: list[str]) -> int: | |
| """ Returns the number of usermod objects in the usermod list """ | |
| # Count the number of entries in the usermods table section | |
| return len([x for x in map_file if ".dtors.tbl.usermods.1" in x]) | |
| def validate_map_file(source, target, env): | |
| """ Validate that all usermods appear in the output build """ | |
| build_dir = Path(env.subst("$BUILD_DIR")) | |
| map_file_path = build_dir / env.subst("${PROGNAME}.map") | |
| if not map_file_path.exists(): | |
| secho(f"ERROR: Map file not found: {map_file_path}", fg="red", err=True) | |
| Exit(1) | |
| # Load project settings | |
| usermods = env.GetProjectOption("custom_usermods","").split() | |
| libdeps = env.GetProjectOption("lib_deps", []) | |
| lib_builders = env.GetLibBuilders() | |
| secho(f"INFO: Expecting {len(usermods)} usermods: {', '.join(usermods)}") | |
| # Map the usermods to libdeps; every usermod should have one | |
| usermod_dirs = [] | |
| for mod in usermods: | |
| modstr = f"{mod} = symlink://" | |
| this_mod_libdeps = [libdep[len(modstr):] for libdep in libdeps if libdep.startswith(modstr)] | |
| if not this_mod_libdeps: | |
| secho( | |
| f"ERROR: Usermod {mod} not found in build libdeps!", | |
| fg="red", | |
| err=True) | |
| Exit(1) | |
| # Save only the final folder name | |
| usermod_dir = Path(this_mod_libdeps[0]).name | |
| # Search lib_builders | |
| this_mod_builders = [builder for builder in lib_builders if Path(builder.src_dir).name == usermod_dir] | |
| if not this_mod_builders: | |
| secho( | |
| f"ERROR: Usermod {mod} not found in library builders!", | |
| fg="red", | |
| err=True) | |
| Exit(1) | |
| usermod_dirs.append(usermod_dir) | |
| # Now parse the map file | |
| map_file_contents = read_lines(map_file_path) | |
| confirmed_usermods = check_map_file_objects(map_file_contents, usermod_dirs) | |
| usermod_object_count = count_registered_usermods(map_file_contents) | |
| secho(f"INFO: {len(usermod_dirs)}/{len(usermods)} libraries linked via custom_usermods, producing {usermod_object_count} usermod object entries") | |
| missing_usermods = set(usermod_dirs).difference(confirmed_usermods) | |
| if missing_usermods: | |
| secho( | |
| f"ERROR: No object files from {missing_usermods} found in linked output!", | |
| fg="red", | |
| err=True) | |
| Exit(1) | |
| return None | |
| Import("env") | |
| env.AddPostAction("$BUILD_DIR/${PROGNAME}.elf", Action(validate_map_file, cmdstr='Checking map file...')) | |
| # Map the usermods to libdeps; every usermod should have one | |
| usermod_dirs = [] | |
| for mod in usermods: | |
| modstr = f"{mod} = symlink://" | |
| this_mod_libdeps = [libdep[len(modstr):] for libdep in libdeps if libdep.startswith(modstr)] | |
| if len(this_mod_libdeps) == 0: | |
| secho( | |
| f"ERROR: Usermod {mod} not found in build libdeps!", | |
| fg="red", | |
| err=True) | |
| Exit(1) | |
| elif len(this_mod_libdeps) > 1: | |
| secho( | |
| f"ERROR: Usermod {mod} resolves to multiple libdeps: {this_mod_libdeps}!", | |
| fg="red", | |
| err=True) | |
| Exit(1) | |
| # Save only the final folder name | |
| usermod_dir = Path(this_mod_libdeps[0]).name | |
| # Search lib_builders | |
| this_mod_builders = [builder for builder in lib_builders if Path(builder.src_dir).name == usermod_dir] | |
| ... |
🧰 Tools
🪛 Ruff (0.11.9)
2-2: sys imported but unused
Remove unused import: sys
(F401)
6-6: platformio.util imported but unused
Remove unused import: platformio.util
(F401)
91-91: Undefined name Import
(F821)
92-92: Undefined name env
(F821)
🤖 Prompt for AI Agents
In pio-scripts/validate_usermods.py around lines 40 to 65, the code exits if a
usermod is not found in libdeps or library builders but does not handle the case
where a usermod resolves to multiple libdeps, which can cause ambiguity. To fix
this, add a check after filtering libdeps for each usermod to detect if multiple
matches exist; if so, log an error indicating multiple libdeps found for that
usermod and exit. This ensures only one unique libdep is associated with each
usermod.
|
i downloaded the new zip with the new changes, just changed in platform.ini
and under but i get this error: |
|
same, still no dice: I have gotten internal temperature UM to compile right, but only once, after deleting .pio folder and .buildcache and doing a full clean. At least now its detecting that UM's are not pulled in properly. Notice the space in the user folder, that has caused trouble before... Then moved back to the original git folder: here it does not compile even if build_chache is disabled. So it really could be some windows folder handling issue... |
my path looks like C:\Users\KrX\Desktop\WLED-main so no spaces but the - but even after removing it, it didnt work. so i compared now a working a not working .pio folder. my findings: libdeps/auto_save.pio-link not working: in all usermods the driveletter is small. and then only the bin and elf are different, and in the broken there was a idedata.json next to the bin with 40k characters which wasnt there in the working pio folder EDIT: im not totally sure, but i took that auto_save.pio-link copied it somewhere else and changed the drive letter to a big C (2 times) and when i hit compile i overwrote that file (since VS Code is mostly faster in rebuilding that metadata) and it worked 3 times. and i didnt use your fix for this |
|
First off, thank you both for jumping right on this! As you've discovered, the last push wasn't a change to the build process, but a validation script to catch any problems at build time and hopefully provide better diagnostics. (And as you can see from the CI results, it doesn't even work in more than a few configurations!) Still it is extremely helpful to see that it is indeed catching the faults on your systems. @KrX3D That's a good catch with the drive letter! That might be why I can't get it to fail on my system -- something about the peculiarities of CWD that is confusing platformio. I will try and replicate this and dig through how platformio handles it -- it could be there's an easy fix in the symlink path generation. |
Not all of our platforms create one by default; ensure it's produced.
The modern linker used with new platforms (ESP8266, ESP32 >v4) always produces paths in the map file with slash; however the old linker for the old ESP32 platform instead produces paths with backslash when building on Windows. Match both types as a path separator when scanning linked symbols.
Ensure all paths used in usermod symlinks are fully resolved, including any case correctness issues on Windows. Apparently PlatformIO does not handle symlink files correctly on Windows if there are case differences between cwd and the resolved path.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pio-scripts/validate_usermods.py (1)
54-74: 🛠️ Refactor suggestionHandle multiple matching libdeps for a usermod.
A previous review comment suggested handling the case where a usermod resolves to multiple libdeps, but this wasn't implemented. This could cause ambiguity in the build process.
modstr = f"{mod} = symlink://" this_mod_libdeps = [libdep[len(modstr):] for libdep in libdeps if libdep.startswith(modstr)] if not this_mod_libdeps: secho( f"ERROR: Usermod {mod} not found in build libdeps!", fg="red", err=True) Exit(1) + elif len(this_mod_libdeps) > 1: + secho( + f"ERROR: Usermod {mod} resolves to multiple libdeps: {this_mod_libdeps}!", + fg="red", + err=True) + Exit(1) # Save only the final folder name usermod_dir = Path(this_mod_libdeps[0]).name
🧹 Nitpick comments (3)
pio-scripts/validate_usermods.py (3)
1-7: Remove unused imports.The static analysis hints correctly identify unused imports that should be removed.
import re -import sys from pathlib import Path # For OS-agnostic path manipulation from click import secho from SCons.Script import Action, Exit -from platformio import util🧰 Tools
🪛 Ruff (0.11.9)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
6-6:
platformio.utilimported but unusedRemove unused import:
platformio.util(F401)
8-11: Missing return type annotation.The
read_linesfunction is missing a return type annotation, unlike other functions in the file.-def read_lines(p: Path): +def read_lines(p: Path) -> list[str]: """ Read in the contents of a file for analysis """ with p.open("r", encoding="utf-8", errors="ignore") as f: return f.readlines()
91-93: Consider more detailed error messages.When the validation fails, it would be helpful to provide more specific guidance to users, especially given the Windows-specific issues mentioned in the PR comments.
secho( - f"ERROR: No object files from {missing_usermods} found in linked output!", + f"ERROR: No object files from {missing_usermods} found in linked output!\n" + f"This may be due to:\n" + f"1. Missing or incorrect libArchive=false setting in the usermod's library.json\n" + f"2. On Windows: issues with symlink paths or drive letter casing\n" + f"3. Build cache issues (try cleaning .pio folder and rebuilding)\n" + f"See issue #4597 for more troubleshooting steps.", fg="red", err=True)🧰 Tools
🪛 Ruff (0.11.9)
91-91: Undefined name
Import(F821)
92-92: Undefined name
env(F821)
92-92: Undefined name
env(F821)
93-93: Undefined name
env(F821)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pio-scripts/load_usermods.py(4 hunks)pio-scripts/validate_usermods.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pio-scripts/load_usermods.py (1)
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.823Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
🪛 Ruff (0.11.9)
pio-scripts/validate_usermods.py
2-2: sys imported but unused
Remove unused import: sys
(F401)
6-6: platformio.util imported but unused
Remove unused import: platformio.util
(F401)
91-91: Undefined name Import
(F821)
92-92: Undefined name env
(F821)
92-92: Undefined name env
(F821)
93-93: Undefined name env
(F821)
pio-scripts/load_usermods.py
10-10: Undefined name env
(F821)
13-13: Undefined name env
(F821)
16-16: Undefined name env
(F821)
16-16: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (12)
pio-scripts/load_usermods.py (8)
5-7: New imports align with the enhanced error handling.The added imports from
click,SCons.Script, andplatformio.builder.tools.piolibsupport the improved error handling and build configuration logic.
10-10: Improved path resolution.Converting
usermod_dirto an absolute path using.resolve()is a good practice that helps avoid relative path issues, especially important given the Windows-specific problems mentioned in the PR comments.🧰 Tools
🪛 Ruff (0.11.9)
10-10: Undefined name
env(F821)
12-15: More efficient initialization.Conditionally initializing
all_usermodsonly when needed (in the "usermods" environment) is more efficient and cleaner than the previous global initialization.🧰 Tools
🪛 Ruff (0.11.9)
13-13: Undefined name
env(F821)
19-19: Added type annotation improves code clarity.Adding the return type annotation to
find_usermodenhances code clarity and maintainability.
36-39: Well-designed WLED module detection.The new
is_wled_moduleutility function provides a clear, centralized way to identify WLED modules, improving code maintainability and readability.
110-121: Core fix for the libArchive issue.This is the key fix for the PR objective. By checking for WLED modules after the ConfigureProjectLibBuilder completes and ensuring libArchive is disabled, the code prevents the settings from being reverted during the build process.
The use of
str()when passing paths toPrependUniquecorrectly addresses the type mismatch issue noted in the retrieved learnings.
122-128: Robust error handling for misconfigured usermods.Adding explicit error detection and reporting for misconfigured usermods (those with libArchive still enabled) is an excellent improvement that will help users identify and fix issues more easily.
1-134: Consider additional debugging for Windows-specific issues.Based on the PR comments, this fix doesn't fully resolve the issues on Windows with VSCode. Consider adding more diagnostic code to trace how PlatformIO handles symlink paths and drive letter casing on Windows.
You could add this diagnostic code to help troubleshoot the Windows-specific issues:
if broken_usermods: broken_usermods = [usermod.name for usermod in broken_usermods] secho( f"ERROR: libArchive=false is missing on usermod(s) {' '.join(broken_usermods)} -- modules will not compile in correctly", fg="red", err=True) Exit(1) + + # Print diagnostic information for Windows-specific debugging + if sys.platform == "win32": + secho("DEBUG: Windows-specific library path information:", fg="yellow") + for dep in result.depbuilders: + if is_wled_module(dep): + src_path = Path(dep.src_dir) + secho(f" Module: {dep.name}", fg="yellow") + secho(f" Source dir: {src_path}", fg="yellow") + secho(f" Drive letter: {src_path.drive}", fg="yellow") + if hasattr(dep, "_libnode") and hasattr(dep._libnode, "symlink_src_path"): + secho(f" Symlink source: {dep._libnode.symlink_src_path}", fg="yellow")This would provide visibility into how paths and symlinks are being handled on Windows systems.
🧰 Tools
🪛 Ruff (0.11.9)
1-1: Undefined name
Import(F821)
10-10: Undefined name
env(F821)
13-13: Undefined name
env(F821)
16-16: Undefined name
env(F821)
16-16: f-string without any placeholders
Remove extraneous
fprefix(F541)
43-43: Undefined name
env(F821)
46-46: Undefined name
env(F821)
47-47: Undefined name
env(F821)
52-52: Undefined name
env(F821)
59-59: Undefined name
env(F821)
71-71: Undefined name
env(F821)
93-93: Undefined name
env(F821)
133-133: Undefined name
env(F821)
pio-scripts/validate_usermods.py (4)
19-22: Robust regex pattern for cross-platform path matching.The regex pattern is well-constructed to handle both forward and backward slashes, making it work on both Unix and Windows systems. This is important given the Windows-specific issues mentioned in the PR comments.
31-34: Simple but effective usermod counting.The
count_registered_usermodsfunction provides a straightforward way to verify that the expected number of usermods are registered in the output.
76-88: Correct logic for detecting missing usermods.The code now correctly uses
set(usermod_dirs).difference(confirmed_usermods)to identify usermods that are present inusermod_dirsbut missing fromconfirmed_usermods, fixing the logic error identified in previous review comments.
91-93: Undefined nameImportandenv.The static analysis correctly identifies that
Importandenvare undefined in this context. However, this is likely a false positive as these are provided by the PlatformIO build system when the script is loaded as an extra script.🧰 Tools
🪛 Ruff (0.11.9)
91-91: Undefined name
Import(F821)
92-92: Undefined name
env(F821)
92-92: Undefined name
env(F821)
93-93: Undefined name
env(F821)
There was a problem hiding this comment.
So it looks like it was, indeed, an issue with pathname case -- I was able to reproduce the problem reliably by spawning VSCode at a terminal with a non-canonical case for the project folder. Updated patch should actually fix it. Note you must either run a "full clean" (not just a regular clean) on your environment, or delete the .pio folder, to unstick a broken build.
I have not yet fixed all the problems with the validate_usermods script -- at the very least, the "all usermods" test environment will still (correctly??) fail because it calls for an ESP8266-only usermod in an ESP32 build. (PlatformIO, apparently, silently discards lib_deps that aren't valid for your current platform; this is probably intended to let projects avoid needing tons of platform-specific environments. I'm not sure I agree if that's the best behaviour.) There is also a potential issue where removed usermods will still "count" in the validation process, though they should be correctly excluded from the final link and shouldn't trip any errors.
|
ok this seems to fix it now ;) now it works every time like it should |
|
Also fixes my problem with my usermod not being added. Nice work! |
|
Please merge when you are happy @willmmiles , don't wait for me |
Use a magic custom_usermods string instead of a magic environment name; and disable the validation script as it triggers on the non- platform-compatible mods.
Remove all the unnecessary bits.
Check the safest possible location for final information on what components are actually being linked in. This demonstrates a safe approach that works even for out-of-tree modules.
Perform validation for external modules, too.
The module instance was missing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pio-scripts/load_usermods.py (1)
82-100: Excellent implementation following previous learningsGreat job implementing the Path to string conversion for
PrependUniquecalls (lines 86, 89) as per the previous review feedback. The new error handling forlibArchiveconfiguration is a significant improvement that will help developers identify misconfigured usermods early in the build process.
🧹 Nitpick comments (3)
pio-scripts/validate_modules.py (2)
2-2: Remove unused importsThese imports are not used in the code and should be removed.
-import sys from pathlib import Path # For OS-agnostic path manipulation-from platformio import util from platformio.builder.tools.piolib import LibBuilderBaseAlso applies to: 7-7
🧰 Tools
🪛 Ruff (0.11.9)
2-2:
sysimported but unusedRemove unused import:
sys(F401)
71-72: Remove unnecessarypassstatementThe
passstatement is unreachable afterExit(1).Exit(1) - passpio-scripts/load_usermods.py (1)
8-8: Remove unused import
LibraryPackageManageris imported but not used in the code.from platformio.builder.tools.piolib import LibBuilderBase -from platformio.package.manager.library import LibraryPackageManager🧰 Tools
🪛 Ruff (0.11.9)
8-8:
platformio.package.manager.library.LibraryPackageManagerimported but unusedRemove unused import:
platformio.package.manager.library.LibraryPackageManager(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pio-scripts/load_usermods.py(3 hunks)pio-scripts/validate_modules.py(1 hunks)platformio.ini(2 hunks)usermods/PWM_fan/setup_deps.py(1 hunks)usermods/seven_segment_display_reloaded/setup_deps.py(1 hunks)usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.cpp(1 hunks)usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- platformio.ini
🧰 Additional context used
🧠 Learnings (1)
pio-scripts/load_usermods.py (1)
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.823Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
🧬 Code Graph Analysis (1)
pio-scripts/load_usermods.py (1)
pio-scripts/validate_modules.py (1)
is_wled_module(11-15)
🪛 Ruff (0.11.9)
usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py
2-2: Undefined name Import
(F821)
4-4: Undefined name env
(F821)
usermods/PWM_fan/setup_deps.py
2-2: Undefined name Import
(F821)
5-5: Undefined name env
(F821)
8-8: Undefined name env
(F821)
10-10: Undefined name env
(F821)
usermods/seven_segment_display_reloaded/setup_deps.py
2-2: Undefined name Import
(F821)
5-5: Undefined name env
(F821)
8-8: Undefined name env
(F821)
pio-scripts/validate_modules.py
2-2: sys imported but unused
Remove unused import: sys
(F401)
7-7: platformio.util imported but unused
Remove unused import: platformio.util
(F401)
93-93: Undefined name Import
(F821)
94-94: Undefined name env
(F821)
94-94: Undefined name env
(F821)
95-95: Undefined name env
(F821)
pio-scripts/load_usermods.py
8-8: platformio.package.manager.library.LibraryPackageManager imported but unused
Remove unused import: platformio.package.manager.library.LibraryPackageManager
(F401)
10-10: Undefined name env
(F821)
37-37: Undefined name env
(F821)
48-48: Undefined name env
(F821)
48-48: Undefined name env
(F821)
48-48: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
🔇 Additional comments (10)
usermods/seven_segment_display_reloaded/setup_deps.py (1)
1-11: LGTM! Clean refactoring to lib_deps approach.The refactoring from
custom_usermodstolib_depswithPackageSpecnormalization is well-implemented. The logic correctly checks for the presence of partner libraries and applies appropriate preprocessor defines.Note: The static analysis warnings about undefined
Importandenvare false positives - these are PlatformIO built-in functions available in the build environment.🧰 Tools
🪛 Ruff (0.11.9)
2-2: Undefined name
Import(F821)
5-5: Undefined name
env(F821)
8-8: Undefined name
env(F821)
10-10: Undefined name
env(F821)
usermods/usermod_v2_rotary_encoder_ui_ALT/setup_deps.py (1)
1-9: LGTM! Consistent refactoring implementation.The script correctly implements the same refactoring pattern seen across other usermod setup scripts. The use of
any()to check for multiple possible library names ("four_line_display_ALT", "usermod_v2_four_line_display_ALT") is a good approach for handling different naming conventions.🧰 Tools
🪛 Ruff (0.11.9)
2-2: Undefined name
Import(F821)
4-4: Undefined name
env(F821)
8-8: Undefined name
env(F821)
usermods/usermod_v2_HttpPullLightControl/usermod_v2_HttpPullLightControl.cpp (1)
7-8: LGTM! Proper usermod registration.The addition of the static instance and
REGISTER_USERMODmacro correctly registers the usermod with the WLED system, following the standard pattern for usermod integration.usermods/PWM_fan/setup_deps.py (1)
7-12:⚠️ Potential issueFix dependency validation logic.
There's a logical issue with the current
elifchain. If both "PWM_fan" and one of the required dependencies ("Temperature" or "sht") are present, the dependency validation for "PWM_fan" will never execute because of theelifstructure.Apply this diff to fix the validation logic:
-if "Temperature" in libs: +if "Temperature" in libs: env.Append(CPPDEFINES=[("USERMOD_DALLASTEMPERATURE")]) -elif "sht" in libs: +elif "sht" in libs: env.Append(CPPDEFINES=[("USERMOD_SHT")]) -elif "PWM_fan" in libs: # The script can be run if this module was previously selected + +if "PWM_fan" in libs and not any(dep in libs for dep in ["Temperature", "sht"]): raise RuntimeError("PWM_fan usermod requires Temperature or sht to be enabled")This ensures the dependency validation runs independently of the other conditions.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Ruff (0.11.9)
8-8: Undefined name
env(F821)
10-10: Undefined name
env(F821)
pio-scripts/validate_modules.py (4)
11-15: LGTM!The function correctly identifies WLED modules by checking if they're located in the usermods directory or have names starting with "wled-".
18-46: Well-implemented utility functionsThe file reading, map file parsing, and usermod counting functions are properly implemented with good error handling and efficient algorithms.
74-91: Excellent validation logicThe module validation implementation provides clear diagnostic output and properly identifies missing modules in the linked output.
93-95: Correct build script integrationThe script properly integrates with the PlatformIO build system by adding the map file generation flag and post-build validation action.
🧰 Tools
🪛 Ruff (0.11.9)
93-93: Undefined name
Import(F821)
94-94: Undefined name
env(F821)
94-94: Undefined name
env(F821)
95-95: Undefined name
env(F821)
pio-scripts/load_usermods.py (2)
13-33: Well-implemented utility functionsGood addition of the return type annotation to
find_usermodand proper implementation ofis_wled_modulethat's consistent with the validation script.
36-48: Excellent simplification of usermod handlingThe new approach of directly appending symlink paths to
lib_depsis cleaner and aligns well with the updated dependency handling mentioned in the PR objectives.🧰 Tools
🪛 Ruff (0.11.9)
37-37: Undefined name
env(F821)
48-48: Undefined name
env(F821)
48-48: Undefined name
env(F821)
48-48: Undefined name
env(F821)
Fix whitespace and remove unused imports
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pio-scripts/validate_modules.py (1)
9-13: Function signature differs from related implementation.The
is_wled_modulefunction here takes anenvparameter, while the similar function inload_usermods.py(lines 28-31) doesn't. Consider aligning the function signatures for consistency across the codebase.-def is_wled_module(env, dep: LibBuilderBase) -> bool: +def is_wled_module(dep: LibBuilderBase, env=None) -> bool: """Returns true if the specified library is a wled module """ - usermod_dir = Path(env["PROJECT_DIR"]).resolve() / "usermods" + if env: + usermod_dir = Path(env["PROJECT_DIR"]).resolve() / "usermods" + else: + # Fallback for compatibility with load_usermods.py usage + usermod_dir = Path.cwd().resolve() / "usermods" return usermod_dir in Path(dep.src_dir).parents or str(dep.name).startswith("wled-")pio-scripts/load_usermods.py (1)
28-31: Consider consistency with validate_modules.py implementation.This
is_wled_modulefunction has a different signature than the one invalidate_modules.py. The logic is identical, but consider whether both should use the same interface for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pio-scripts/load_usermods.py(3 hunks)pio-scripts/validate_modules.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pio-scripts/load_usermods.py (1)
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/load_usermods.py:108-119
Timestamp: 2025-05-01T01:55:12.823Z
Learning: When using SCons' PrependUnique method with Path objects, convert them to strings first using str() to avoid type mismatches, as PrependUnique treats Path and str as distinct objects.
🧬 Code Graph Analysis (1)
pio-scripts/validate_modules.py (1)
pio-scripts/load_usermods.py (1)
is_wled_module(28-31)
🪛 Ruff (0.11.9)
pio-scripts/load_usermods.py
8-8: Undefined name env
(F821)
35-35: Undefined name env
(F821)
46-46: Undefined name env
(F821)
46-46: Undefined name env
(F821)
46-46: Undefined name env
(F821)
pio-scripts/validate_modules.py
90-90: Undefined name Import
(F821)
91-91: Undefined name env
(F821)
91-91: Undefined name env
(F821)
92-92: Undefined name env
(F821)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
🔇 Additional comments (9)
pio-scripts/validate_modules.py (2)
90-92: SCons integration correctly uses standard patterns.The usage of
Import("env")and the subsequent SCons integration follows standard patterns. The static analysis warnings about undefined names are false positives - these are SCons-specific globals.🧰 Tools
🪛 Ruff (0.11.9)
90-90: Undefined name
Import(F821)
91-91: Undefined name
env(F821)
91-91: Undefined name
env(F821)
92-92: Undefined name
env(F821)
31-31:✅ Verification successful
Verify regex pattern handles Windows path separators correctly.
Given the PR context mentions Windows-specific issues with drive letter casing and path handling, ensure the regex pattern correctly handles both forward slashes and backslashes in paths.
🏁 Script executed:
#!/bin/bash # Test the regex pattern with sample Windows and Unix paths python3 -c " import re pattern = re.compile(r'0x0*[1-9a-f][0-9a-f]*\s+0x[0-9a-f]+\s+\S+[/\\\\](test_dir)[/\\\\]\S+\.o') test_cases = [ '0x00001234 0x00000100 C:\\\\path\\\\to\\\\test_dir\\\\file.o', '0x00001234 0x00000100 /path/to/test_dir/file.o', '0x00001234 0x00000100 c:\\\\path\\\\to\\\\test_dir\\\\file.o', # lowercase drive '0x00001234 0x00000100 ./build/test_dir/file.o' ] for test in test_cases: match = pattern.findall(test) print(f'Input: {test}') print(f'Match: {match}') print('---') "Length of output: 876
Regex correctly handles both Windows and Unix paths
The existingobject_path_regexhas been verified against sample inputs—including backslashes, forward slashes, uppercase and lowercase drive letters—and matches as expected. No changes needed.pio-scripts/load_usermods.py (7)
4-6: New imports support enhanced error reporting.The addition of
click,SCons.Script.Exit, andLibBuilderBaseimports enables colored error output and proper build termination, which aligns well with the validation functionality added later in the file.
8-8: Good practice using resolve() for absolute paths.Using
resolve()ensures the usermod directory path is absolute, which should help address the Windows path-related issues mentioned in the PR comments.🧰 Tools
🪛 Ruff (0.11.9)
8-8: Undefined name
env(F821)
38-42: Improved usermod expansion logic.The wildcard expansion logic is more robust, properly filtering for directories that contain
library.jsonfiles. This should help ensure only valid usermods are included when using the "*" option.
45-46: Transition to lib_deps approach is well-implemented.The change from custom environment configuration to using
lib_depswith symlinks should help address the core issue described in the PR objectives. The symlink paths are properly resolved to absolute paths.🧰 Tools
🪛 Ruff (0.11.9)
46-46: Undefined name
env(F821)
46-46: Undefined name
env(F821)
46-46: Undefined name
env(F821)
84-87: Correct usage of str() with PrependUnique.Good application of the learning from previous reviews - using
str()when callingPrependUniquewith Path objects to avoid type mismatches.
89-98: Robust libArchive validation with clear error reporting.The validation logic properly checks for the
libArchivesetting and provides clear, colored error messages when misconfigured usermods are detected. The use ofExit(1)ensures the build fails fast when issues are found.
80-90:❓ Verification inconclusive
Verify libArchive property access pattern.
Based on previous feedback,
dep.lib_archiveis a read-only property. Confirm this is the correct way to check the libArchive setting after the manifest changes.
🏁 Script executed:
#!/bin/bash # Check how lib_archive property is implemented in PlatformIO's LibBuilderBase rg -A 10 -B 5 "lib_archive.*property\|def lib_archive" --type pyLength of output: 66
Confirm
lib_archiveProperty UsageThis property is defined in PlatformIO Core (not in this repo) and should be a read-only attribute reflecting the library archive status after the manifest changes. Please verify that:
dep.lib_archivestill exists as a read-only property on the builder class- It correctly returns a truthy value when an archive is present
- This check (
if dep.lib_archive:) remains valid following the manifest refactorLocations to review:
- pio-scripts/load_usermods.py, around lines 86–90 (the
if dep.lib_archive:block)
| def read_lines(p: Path): | ||
| """ Read in the contents of a file for analysis """ | ||
| with p.open("r", encoding="utf-8", errors="ignore") as f: | ||
| return f.readlines() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for file operations.
The file reading function should handle potential I/O errors more robustly.
def read_lines(p: Path):
""" Read in the contents of a file for analysis """
- with p.open("r", encoding="utf-8", errors="ignore") as f:
- return f.readlines()
+ try:
+ with p.open("r", encoding="utf-8", errors="ignore") as f:
+ return f.readlines()
+ except (OSError, IOError) as e:
+ secho(f"ERROR: Failed to read file {p}: {e}", fg="red", err=True)
+ Exit(1)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pio-scripts/validate_modules.py around lines 16 to 19, the read_lines
function currently opens and reads a file without handling potential I/O errors.
Update the function to include try-except blocks that catch exceptions like
FileNotFoundError and IOError, and handle them appropriately, such as logging an
error message or returning an empty list, to make the file reading operation
more robust.
There was a problem hiding this comment.
In this use case, a file read error indicates something deeply wrong on the user's build platform, since the script itself adds the required parameters to produce the only file we open. While it's definitely possible to add more sophisticated error messages, I don't judge that it's worth the extra code (and future maintainence) given that the only way to get there is either a hardware failure on the user's build platform or they're messing around with unsupported build options.
There was a problem hiding this comment.
@willmmiles Thank you for the clarification! That makes perfect sense - since the script itself adds the linker flags to generate the map file, a file read error would indeed indicate a fundamental build system issue rather than a typical I/O error. Your cost-benefit analysis is spot-on; adding error handling for what would essentially be unrecoverable platform failures isn't worth the maintenance overhead.
I appreciate you taking the time to explain the context and design rationale.
(\ /)
( ._.)
o_(")(")
✏️ Learnings added
Learnt from: willmmiles
PR: wled/WLED#4669
File: pio-scripts/validate_modules.py:16-19
Timestamp: 2025-05-27T02:17:37.862Z
Learning: In pio-scripts/validate_modules.py, the read_lines function doesn't include error handling because the script itself generates the map file it reads via linker flags. File read errors would indicate fundamental build platform issues or unsupported modifications, making additional error handling not worth the maintenance overhead according to the author willmmiles.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
The ConfigureProjectLibBuilder process will flush and reload the library settings from the on-disk manifests if any new library is added to the build at that stage. This reverts the libArchive setting change performed prior to that call. Apply the setting afterwards, instead.
Fixes #4597
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores